Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Reviewer's GuideAdds Merkle-tree-based wrapped signature support to IthacaAccount, extends the wrapped signature format with a merkle flag, updates all tests and helpers to the new encoding, and introduces fuzz tests for Merkle proofs, while slightly simplifying EIP-712 domain usage and bumping the account version. Sequence diagram for wrapped and Merkle signature validationsequenceDiagram
actor Caller
participant IthacaAccount
participant MerkleProofLib
participant ECDSA
Caller->>IthacaAccount: unwrapAndValidateSignature(digest, signature)
activate IthacaAccount
IthacaAccount->>IthacaAccount: Check if contract signature
alt Contract signature
IthacaAccount->>ECDSA: recoverCalldata(digest, signature)
ECDSA-->>IthacaAccount: recoveredAddress
IthacaAccount-->>Caller: isValid, keyHash = 0,1 or 0,0
else Wrapped signature
IthacaAccount->>IthacaAccount: Extract keyHash, prehash flag, merkle flag
alt prehash flag is set
IthacaAccount->>IthacaAccount: digest = sha2(digest)
end
alt merkle flag is set
IthacaAccount->>IthacaAccount: _verifyMerkleSig(digest, innerSignature)
activate IthacaAccount
IthacaAccount->>MerkleProofLib: verifyCalldata(proof, root, digest)
MerkleProofLib-->>IthacaAccount: proofIsValid
alt proofIsValid
IthacaAccount->>IthacaAccount: unwrapAndValidateSignature(root, rootSig)
IthacaAccount-->>IthacaAccount: isValid, keyHash
IthacaAccount-->>Caller: isValid, keyHash
else invalid proof
IthacaAccount-->>Caller: false, 0
end
deactivate IthacaAccount
else non-merkle wrapped signature
IthacaAccount->>IthacaAccount: getKey(keyHash)
IthacaAccount-->>Caller: isValid, keyHash
end
end
deactivate IthacaAccount
Class diagram for IthacaAccount signature verification changesclassDiagram
class IthacaAccount {
+unwrapAndValidateSignature(digest bytes32, signature bytes) bool isValid, bytes32 keyHash
-_verifyMerkleSig(digest bytes32, signature bytes) bool isValid, bytes32 keyHash
+getEIP712NameAndVersion() string name, string version
}
class MerkleProofLib {
+verifyCalldata(proof bytes32[], root bytes32, leaf bytes32) bool
}
class LibBytes {
+loadCalldata(data bytes, offset uint256) bytes32
+truncatedCalldata(data bytes, newLength uint256) bytes
}
class EfficientHashLib {
+sha2(input bytes32) bytes32
}
IthacaAccount ..> MerkleProofLib : uses
IthacaAccount ..> LibBytes : uses
IthacaAccount ..> EfficientHashLib : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements Merkle signature verification in IthacaAccount.sol, enabling the validation of digests against a Merkle root signed by an authorized key. The changes include a new _verifyMerkleSig internal function, updates to the signature format to include a merkle flag, and comprehensive fuzz tests. Review feedback identifies a security vulnerability in the assembly block of _verifyMerkleSig due to missing length and offset validations for calldata reads. Additionally, it is recommended to reorder the loading of signature flags in unwrapAndValidateSignature to avoid reading from a truncated slice, ensuring better code robustness.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The manual ABI decoding in
_verifyMerkleSigassumes well‑formedabi.encode(proof, root, rootSig)calldata; consider adding minimal sanity checks onsignature.lengthand the computed offsets to avoid out‑of‑bounds reads on malformed input. - In
unwrapAndValidateSignature, whenmerkleis true the previously decodedkeyHashis ignored; it may be clearer to either validate that it is zero or explicitly document thatkeyHashis derived solely from the nestedrootSigin the Merkle case.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The manual ABI decoding in `_verifyMerkleSig` assumes well‑formed `abi.encode(proof, root, rootSig)` calldata; consider adding minimal sanity checks on `signature.length` and the computed offsets to avoid out‑of‑bounds reads on malformed input.
- In `unwrapAndValidateSignature`, when `merkle` is true the previously decoded `keyHash` is ignored; it may be clearer to either validate that it is zero or explicitly document that `keyHash` is derived solely from the nested `rootSig` in the Merkle case.
## Individual Comments
### Comment 1
<location path="src/IthacaAccount.sol" line_range="546-554" />
<code_context>
}
+ bool merkle;
unchecked {
- uint256 n = signature.length - 0x21;
+ uint256 n = signature.length - 0x22;
keyHash = LibBytes.loadCalldata(signature, n);
signature = LibBytes.truncatedCalldata(signature, n);
// Do the prehash if last byte is non-zero.
if (uint256(LibBytes.loadCalldata(signature, n + 1)) & 0xff != 0) {
digest = EfficientHashLib.sha2(digest); // `sha256(abi.encode(digest))`.
}
+ merkle = uint256(LibBytes.loadCalldata(signature, n + 2)) & 0xff != 0;
+ }
+
</code_context>
<issue_to_address>
**issue (bug_risk):** The offsets used for the `prehash` and `merkle` flags appear incorrect relative to the keyHash position.
With `abi.encode(bytes(innerSignature), bytes32(keyHash), bool(prehash), bool(merkle))` and `uint256 n = signature.length - 0x22`, `n` marks the start of the 32‑byte `keyHash`, so the flags must be at `n + 32` and `n + 33` (0x20 / 0x21), not `n + 1` and `n + 2`. As implemented, both flags are read from within the `keyHash` bytes, so the decoded `prehash`/`merkle` values will be wrong. Consider defining `keyHashStart`, `prehashOffset`, and `merkleOffset` constants to make this layout explicit and avoid such off‑by‑N issues.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Introduce Merkle tree–based wrapped signature support and extend the account signature format to distinguish Merkle and prehashed signatures.
New Features:
Enhancements:
Tests: